Use khiops-core and khiops-driver-* pip packages containing binaries only#582
Use khiops-core and khiops-driver-* pip packages containing binaries only#582tramora wants to merge 2 commits into
Conversation
tramora
force-pushed
the
core-and-drivers-as-pip-packages
branch
9 times, most recently
from
caa2f18 to
fd7c550
Compare
tramora
force-pushed
the
core-and-drivers-as-pip-packages
branch
2 times, most recently
from
394d474 to
5312426
Compare
|
False sentiment of success : we simulate multiple Python environments using conda (#573). This is very unfortunate as we are not able yet to detect automatically an issue with the new PyPI packages regarding the remote drivers khiops-core:1041. In my opinion this one is a blocking issue. |
tramora
force-pushed
the
core-and-drivers-as-pip-packages
branch
from
5312426 to
b0223fa
Compare
Hence, to properly test this, issue #573 must be addressed first. Right, @tramora ? |
tramora
force-pushed
the
core-and-drivers-as-pip-packages
branch
from
95a1d21 to
cfd1802
Compare
I changed the |
| an absolute path is inferred using the current folder which will probably | ||
| cause an error eventually. | ||
|
|
||
| this method raises a `RuntimeError` if the dynamic library is not found |
There was a problem hiding this comment.
Reformat this line to:
Raises
-------
`RuntimeError`
The dynamic library of the driver is not found.
There was a problem hiding this comment.
s/the dynamic library/The dynamic library/ (capital "T").
tramora
force-pushed
the
core-and-drivers-as-pip-packages
branch
2 times, most recently
from
4a72d1f to
db651cb
Compare
…ries only
- these packages become mandatory ("khiops-core") or optional ("khiops-driver-*") dependencies dragged during the installation process
- these packages must be installed in the Conda environments used to simulate multiple environs (the Conda packages MUST not be used)
- in order to avoid distorting usage statistics the test workflows will always use khiops packages from TestPypi
tramora
force-pushed
the
core-and-drivers-as-pip-packages
branch
from
db651cb to
ad82998
Compare
| `Dockerfile.ubuntu-debian` is a unique Dockerfile to build either a Ubuntu or a Debian target image | ||
| because there is no difference except for the base image used. | ||
|
|
||
| The only reason why the system-wide native Khiops binary (from a `.deb` package) is still installed in the target image |
There was a problem hiding this comment.
IMHO, we can leave that restriction aside if we just install via Pip in a virtualenv, then set the KHIOPS_CMD and KHIOPS_COCLUSTERING_CMD environment variables accordingly. See: https://github.com/KhiopsML/khiops-server/blob/7050d79fb608c59eed4831838d3753867a2a80f6/server/server.go#L63 and https://github.com/KhiopsML/khiops-server/blob/7050d79fb608c59eed4831838d3753867a2a80f6/server/server.go#L70. I would set these in the CI where applicable.
| type: string | ||
| default: 11.0.0 | ||
| description: Khiops Revision | ||
| description: Khiops Revision (for tests against KhiopsDockerRunner) |
There was a problem hiding this comment.
I'd use virtualenv and set KHIOPS_CMD and KHIOPS_COCLUSTERING_CMD (see the previous comment).
| Raises | ||
| ------ | ||
| `RuntimeError` | ||
| the dynamic library of the driver is not found |
There was a problem hiding this comment.
Use capital "T" in "the dynamic".
| khiops_drivers_path = os.environ.get("KHIOPS_DRIVERS_PATH") | ||
| if khiops_drivers_path: | ||
| _check_khiops_driver_library( | ||
| remote_storage_type="azure", khiops_drivers_path=khiops_drivers_path | ||
| ) | ||
|
|
There was a problem hiding this comment.
I'm not sure we should access KHIOPS_DRIVERS_PATH directly. Normally, this environment variable name should be abstracted away by the KhiopsLocalRunner. I would create specific attribute for accessing it instead, e.g. khiops_drivers_path, akin to khiops_path and khiops_coclustering_path.
| in the python system folder | ||
| or in the Python folder inside the home directory of the user, | ||
| or in a virtual environment (if one is used) | ||
| - 'pip' environment containing binaries, shared libraries and the python libraries |
| - 'pip' environment containing binaries, shared libraries and the python libraries | ||
| can either be : | ||
| - system-wide (strongly discouraged) | ||
| - or in the Python folder inside the home directory of the user (aka "User site"), |
| "Make sure you have installed properly the Khiops Python library " | ||
| "and Khiops (if the latter was not installed automatically). " |
There was a problem hiding this comment.
I would simplify a bit to just:
"Make sure you have installed Khiops properly."
Actually, the whole point of the "full Pip" installation route (alongside the "full Conda" one) is to abstract away the "Khiops Python library" / "Khiops binaries" dichotomy (which can be kept in the developer comments though, but not in the user-facing warnings, messages or feedback IMHO).
There was a problem hiding this comment.
I'm not very comfortable with the vague "Khiops" word as we show messages to the end user.
Indeed the user is using "The Khiops Python Library" so it is his/her starting point that why I would like to be precise on this point but I will follow
There was a problem hiding this comment.
Ok, then we could keep "Make sure you have installed the Khiops Python library correctly.", without mentioning the "and Khiops" and the latter being installed differently, because it would be out of the scope of the "full Pip" installation IMHO.
On the other hand, if a user deliberately chooses to uninstall the khiops-core package in order to replace the binaries with, say, a locally-built Khiops Core set of binaries, then that user would not need this kind of warning in the first place, IMHO.
| "The 'khiops_env' script not found for the current " | ||
| f"'{installation_method}' installation method. Make sure " | ||
| "you have installed khiops >= 10.2.3. " | ||
| "you have installed properly the Khiops Python library " |
There was a problem hiding this comment.
Simplify to "you have installed Khiops properly." (see relevant comment above).
| # On Windows "KHIOPS_MPI_DLL_PATH" (containing the Intel MPI Library) | ||
| # must be added to "PATH" otherwise Khiops wouldn't find it | ||
| # and fail immediately | ||
| elif var_name == "KHIOPS_MPI_DLL_PATH": |
There was a problem hiding this comment.
This must only be done for Pip installations (whence not for Conda). Cf. https://github.com/KhiopsML/khiops/blob/262e2c66788d63eb375c53e9301a283ac3cea82f/packaging/windows/khiops_env.cmd.in#L22.
| "See https://khiops.org for more information.", | ||
| ) | ||
|
|
||
| # Warn if the khiops minor and patch versions do not match |
There was a problem hiding this comment.
I'm not sure all this still applies: if khiops-python is installed, then khiops-core is installed. If khiops-core is uninstalled by the user afterwards, then the environment initialization fails.
There was a problem hiding this comment.
At least during the making of the current PR, the case occurs :
- Khiops is 11.0.1rc1
- The Khiops Library is 11.0.0.3 (in the PR branch)
Surely the khiops team won't let this occur in the final release but who knows ?
| compatible_khiops_version = khiops.get_compatible_khiops_version() | ||
|
|
||
| operating_system = platform.system() | ||
| installation_method = _infer_khiops_installation_method() |
There was a problem hiding this comment.
I would remove all code after self._khiops_version, because the version mismatch would no longer be relevant here: it would be given by the package metadata (Conda or Pip) - see the comment below as well.
| LABEL description="Container for the development of khiops-python" | ||
|
|
||
| # Install dev tools and miniforge (for the unit tests); build and install Khiops | ||
| # Install dev tools and miniforge (for the unit tests); install Khiops (only for tests against KhiopsDockerRunner) |
There was a problem hiding this comment.
I would drop this and set the KHIOPS_CMD and KHIOPS_COCLUSTERING_CMD to point to the specific Pip installation in the CI (see the other comments in this respect).
| # Get Linux distribution codename \ | ||
| && if [ -f /etc/os-release ]; then . /etc/os-release; fi \ | ||
| # Obtain the Khiops native package \ | ||
| && KHIOPS_PKG_FILE=$KHIOPS_REVISION/khiops-core-openmpi_$KHIOPS_REVISION-1-$VERSION_CODENAME.amd64.deb \ | ||
| && wget -O KHIOPS_CORE.deb "https://github.com/KhiopsML/khiops/releases/download/${KHIOPS_PKG_FILE}" \ | ||
| # Install the Khiops native package : make it always succeed. \ | ||
| # If dpkg fails it is due to missing dependencies which will be installed by apt in the next line \ | ||
| && (dpkg -i --force-all KHIOPS_CORE.deb || true) \ | ||
| && apt-get -f -y install \ | ||
| && rm -f KHIOPS_CORE.deb \ |
There was a problem hiding this comment.
Drop this altogether and have the Docker runner use the Pip installation in the CI (see the other comments in this respect).
|
|
||
| # Install dev tools and miniforge (for the unit tests); build and install Khiops | ||
| # Install dev tools and miniforge (for the unit tests); install Khiops (only for tests against KhiopsDockerRunner) | ||
| ARG KHIOPS_REVISION |
There was a problem hiding this comment.
Drop this and use virtualenv in the CI (see the other comments in this respect).
popescu-v
left a comment
popescu-v
left a comment
There was a problem hiding this comment.
See pending comments.
2 participants
Fixes #572 and #578
TODO Before Asking for a Review
main(ormain-v10)Unreleasedsection ofCHANGELOG.md(no date)index.html